Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds google_cloud_run_service data source #7388

Merged
merged 2 commits into from
Oct 2, 2020
Merged

adds google_cloud_run_service data source #7388

merged 2 commits into from
Oct 2, 2020

Conversation

kamaz
Copy link
Contributor

@kamaz kamaz commented Sep 29, 2020

Creates a data source for cloud run service as mentioned in #7363

Would be good to get the early feedback.

When it comes to documentation for time being, I left TBD and still some references to Cloud Function as I used that for the template. For documentation object reference to do you copy text from resources or do you just link, what is the preferred way?

Closes: #7363

cloudrun: Add support for a `google_cloud_run_service` datasource

@ghost ghost added size/m labels Sep 29, 2020
@ghost ghost requested a review from rileykarson September 29, 2020 21:21
Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! I've just got a few comments, mostly on the docs.

func dataSourceGoogleCloudRunService() *schema.Resource {

dsSchema := datasourceSchemaFromResourceSchema(resourceCloudRunService().Schema)
addRequiredFieldsToSchema(dsSchema, "name", "location")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to addOptionalFieldsToSchema for project as well, so it can be configured by the user.

* `project` - (Optional) The project in which the resource belongs. If it
is not provided, the provider project is used.

* `region` - (Optional) The region in which the resource belongs. If it
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be omitted, since it isn't a field on this resource (likely just a leftover?)


## Attributes Reference

TBD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've got it right, imo- we should explicitly document the configurable parts and then link the resource documentation for the attributes.

@ghost ghost added size/l and removed size/m labels Sep 30, 2020
@kamaz
Copy link
Contributor Author

kamaz commented Sep 30, 2020

hi @rileykarson, thanks for the comments I think I've addressed all now. Also, I've extended the test coverage to ensure project is optional.

Please let me know if there are more changes needed.

Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

I'm going to upstream this to Magic Modules (this repo is generated from there) before merging, so there'll be a slight delay, probably just a few hours, between this approving & merging.

@ghost
Copy link

ghost commented Nov 2, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloud Run Data Sources
2 participants